The HA transaction is rolled back correctly with:

• Abort HA Realization Logic After Timeout #800

This is the proof that we actually leak database connections but only for as long as the context is not cancelled.

package main

import (
	"context"
	"github.com/icinga/icinga-go-library/logging"
	"github.com/icinga/icingadb/internal/command"
	"go.uber.org/zap"
	"log"
	"time"
)

func main() {
	cmd := command.New()

	db, err := cmd.Database(logging.NewLogger(zap.NewNop().Sugar(), time.Second))
	if err != nil {
		log.Fatal(err)
	}
	db.SetMaxOpenConns(3)

	go func() {
		time.Sleep(1 * time.Second)

		ctx, _ := context.WithDeadline(context.Background(), time.Now().Add(10*time.Second))
		_, _ = db.BeginTxx(ctx, nil)
		return
	}()
	go func() {
		time.Sleep(1 * time.Second)

		ctx, _ := context.WithDeadline(context.Background(), time.Now().Add(10*time.Second))
		_, _ = db.BeginTxx(ctx, nil)
		return
	}()

	time.Sleep(3 * time.Second)

	ctx, cancelFunc := context.WithDeadline(context.Background(), time.Now().Add(2*time.Second))
	defer cancelFunc()
	_ = db.PingContext(ctx)
	log.Printf("OpenConnections: %v", db.Stats().OpenConnections)
	log.Printf("InUse: %v", db.Stats().InUse)
}

~/Workspace/go/icingadb (set-ha-conn-limit ✗) go run ./cmd/decoder/decoder.go --config config.example.yml
2024/10/23 16:19:59 OpenConnections: 3
2024/10/23 16:19:59 InUse: 2

Additionally, this masked a serious bug in the HA#realize() method, where we start a new transaction after each retry without rolling back in case of an error, leading to connections not being released before exceeding the ctx deadline.

This bug is being addressed in #800. In spite of other changes over there, I am a bit critical about reducing the amount of database connections to a single one. Currently, there is still the possibility of a long-running COMMIT to the database which cannot be canceled, as described in this comment. Could such a scenario then result into a blocking state if there is just one connection available? I guess no, but my gut instinct is uncertain.

Additionally, this masked a serious bug in the HA#realize() method, where we start a new transaction after each retry without rolling back in case of an error, leading to connections not being released before exceeding the ctx deadline.

This bug is being addressed in #800. In spite of other changes over there, I am a bit critical about reducing the amount of database connections to a single one. Currently, there is still the possibility of a long-running COMMIT to the database which cannot be canceled, as described in this comment. Could such a scenario then result into a blocking state if there is just one connection available? I guess no, but my gut instinct is uncertain.

With a long-running commit and more than one database connection, successive transactions would be blocked during the SELECT. With only one database connection, the code waits at the begin transaction point. To me this is a valid change as we are not opening new database connections unnecessarily. But since PR #800 was merged, the PR and commit description should reflect that this change is only about that. @yhabteab Would you please adjust that accordingly?

@yhabteab With both MySQL and PostgreSQL integration tests failing, I think something is wrong 😆

I followed @yhabteab's statement that this PR breaks when used together with #800 and tried a bisect with this PR's change on each commit of my PR. Indeed, dd0ca8f broke it. Let me debug it further :)

$ git bisect start HEAD HEAD~6 --
Bisecting: 2 revisions left to test after this (roughly 2 steps)
[f8819208ce4856a83d627d703f749e040c4d2888] HA: Deferred SQL Transaction Rollback


$ cat 828.sh
#!/bin/sh

set -eux

git cherry-pick --no-commit set-ha-conn-limit

CGO_ENABLED=0 go build ./cmd/icingadb

pushd tests
go test -o ../icingadb-test -c .
popd

set +e
export ICINGADB_TESTS_DATABASE_TYPE="mysql"
export ICINGA_TESTING_ICINGADB_BINARY="$(pwd)/icingadb"
export ICINGA_TESTING_ICINGADB_SCHEMA_MYSQL="$(pwd)/schema/mysql/schema.sql"
./icingadb-test -icingatesting.debuglog debug.log -test.v -test.run TestCleanupAndRetention
RET="$?"
set -e

git restore --staged cmd/icingadb/main.go
git restore cmd/icingadb/main.go

exit "$RET"


$ git bisect run ./828.sh
[ . . . ]
dd0ca8fb072ae8713b7e3f32df5b6417aa0fb7b6 is the first bad commit
commit dd0ca8fb072ae8713b7e3f32df5b6417aa0fb7b6
Author: Alvar Penning <alvar.penning@icinga.com>
Date:   Fri Oct 25 10:39:05 2024 +0200

    HA: Insert environment within retryable function

    The HA.insertEnvironment() method was inlined into the retryable
    function to use the deadlined context. Otherwise, this might block
    afterwards, as it was used within HA.realize(), but without the passed
    context.

 pkg/icingadb/ha.go | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)
bisect found first bad commit

After a bit of debugging together with @yhabteab, we have identified a bug and drafted #878.

@yhabteab: Could you please rebase again. Thanks :)

@yhabteab: Could you please rebase again. Thanks :)

Already did.